-
Notifications
You must be signed in to change notification settings - Fork 13
feat: Result type for ComposablePasses #2703
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Agustín Borgna <[email protected]>
68df8b0 to
567c58d
Compare
567c58d to
d79a031
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2703 +/- ##
==========================================
+ Coverage 83.49% 83.54% +0.05%
==========================================
Files 266 266
Lines 51744 51667 -77
Branches 47180 47086 -94
==========================================
- Hits 43205 43167 -38
+ Misses 6161 6124 -37
+ Partials 2378 2376 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CalMacCQ
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks nice, thanks. I find this more intuntive than having _apply and _apply_inplace methods (at least one of which must be overriden when the protocol is implemented.
One question for clarification and also one nit.
| class ComposablePass(Protocol): | ||
| """A Protocol which represents a composable Hugr transformation.""" | ||
|
|
||
| def __call__(self, hugr: Hugr, *, inplace: bool = True) -> Hugr: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have the impl_pass_run function as a helper for implementing ComposablePass.run where is the __call__ method actually used in the pass implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a suggestion from Seyon. Most usecases just need the Hugr after the pass, so we provide a simple call method.
When we actually need to inspect the result/pass output we can use the other call.
fce9705 to
07caa46
Compare
Co-authored-by: Alan Lawrence <[email protected]>
| hugr=hugr, | ||
| inplace=inplace, | ||
| inplace_call=apply_inplace, | ||
| inplace_call=apply, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inplace_call and copy_call are the same function here, is that right? Either of them will capture local variable inplace: bool = True specified in the call to run.
Ah ok. I think the one that's going to be used will have the correct semantics, and the callback that isn't going to be used....had better not be, it will not work correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added explicit flag overrides
| :attr hugr: The transformed Hugr. | ||
| :attr original_dirty: Whether the original HUGR was modified by the pass. | ||
| :attr modified: Whether the pass made changes to the HUGR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so if inplace = True,
original_dirty==modified, and the transformed Hugristhe input Hugr
whereas if inplace == False,
original_dirtyshould always beFalsemodifiedindicates whether the output Hugr == the input- (probably but not necessarily) the output never
isthe input. (We might want to allow the latter ifmodifiedisFalse)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified indicates whether the output Hugr == the input; but (probably, not necessarily) the output never is the input. (We might want to allow the latter if modified is False)
Conditionally aliasing the output is a bug waiting to happen. If we say the output is a copying the object then we should always do that.
Otherwise we may start modifying the result and unintentionally changing the original too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Ok, so - if inplace:
original_dirty==modified- output Hugr
isinput Hugr
if not inplace:
original_dirty == False? (Right? We don't allow it to be partially invalidated or anything)- (output Hugr
isinput Hugr) is alwaysFalse modifiedindicates whether the output Hugr==the input
|
|
||
|
|
||
| def impl_pass_call( | ||
| def impl_pass_run( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me this name feels too "internal". I know it's inside an underscored module but when we stabilize it we may want a better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed it to "implement_pass_run".
Not the best either, but hopefully a bit less internal-sounding
| modify the program. | ||
| :attr hugr: The transformed Hugr. | ||
| :attr original_dirty: Whether the original HUGR was modified by the pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "dirty" the right word here? The meaning of original_dirty and modified look very similar. I don't actually see the point of the original_dirty flag. Presumably the user either knows or doesn't care whether the pass was called in-place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced original_dirty with just an inplace flag.
I think it's nice to have a flag assuring you whether the hugr got copied or not, but it's not super important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. We also rule out any optimization that wants to destroy the input Hugr in the process of constructing an all-new output Hugr. I mean, ruling that out might be good ;)
| hugr: Hugr | ||
| original_dirty: bool = False | ||
| modified: bool = False | ||
| results: list[tuple[PassName, Any]] = field(default_factory=list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we include the pass name here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly for debuggability, otherwise ComposedPass.run would return a List[Any] of arbitrary payloads without much indication of what came from where.
The result may also be serialized, so the original ComposedPass and its list of passes may be lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought including the pass name was quite a nice solution!
| else: | ||
| self.passes.append(composable_pass) | ||
|
|
||
| def run(self, hugr: Hugr, *, inplace: bool = True) -> PassResult: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think you can just do:
def run(self, hugr: Hugr, *, inplace: bool = True) -> PassResult:
pass_result = PassResult(hugr=hugr)
for pass_ in self.passes:
new_result = pass_.run(pass_result.hugr, inplace=inplace)
pass_result = pass_result.then(new_result)
return pass_result
Because pass_.run always returns a PassResult containing the result hugr; we don't care whether that's the input hugr or a fresh one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's already the case, or I'm missing something?
Includes an idea for simplifying the protocol's `_apply`/`_apply_inline` from #2697 by providing a helper function instead (859c811). --------- Co-authored-by: Callum Macpherson <[email protected]> Co-authored-by: Callum Macpherson <[email protected]> Co-authored-by: Alan Lawrence <[email protected]>
Includes an idea for simplifying the protocol's
_apply/_apply_inlinefrom #2697 by providing a helper function instead (859c811).